-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
write trace to file as it comes #635
Conversation
The changes in the GIT_DIFF primarily revolve around how traces are handled and how output directories are managed. Notably, changes include:
Concerns:
Suggested changes: + async function setupTraceWriting(trace: Trace, filename: string) {
+ await ensureDir(dirname(filename))
+ await writeFile(filename, "", { encoding: "utf-8" })
+ trace.addEventListener(
+ TRACE_CHUNK,
+ async (ev) => {
+ const tev = ev as TraceChunkEvent
+ await appendFile(filename, tev.chunk, { encoding: "utf-8" })
+ },
+ false
+ )
+ }
+ if (out) {
+ if (removeOut) await emptyDir(out)
+ await ensureDir(out)
+ }
+ if (out && trace) {
+ const ofn = join(out, "res.trace.md")
+ if (outTrace) {
+ setupTraceWriting(trace, outTrace);
+ }
+ if (ofn !== outTrace) {
+ setupTraceWriting(trace, ofn);
+ }
+ } Overall, the changes look fine except for the noted concerns and suggested improvements. The change prioritizes writing trace chunks as they arrive instead of at the end, which could be beneficial for long-running processes. However, make sure to test these changes thoroughly to ensure that the trace writing functionality works as expected. Concerns aside, LGTM 🚀
|
await appendFile(filename, tev.chunk, { encoding: "utf-8" }) | ||
}, | ||
false | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using an async function as an event listener can lead to unhandled promise rejections if the promise is rejected. Consider wrapping the async function call within the event listener in a try-catch block to handle any potential errors. 🛠️
generated by pr-review-commit
async_in_event_listener
await setupTraceWriting(trace, ofn) | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setup for trace writing is only done if 'out' and 'trace' are truthy. However, 'outTrace' is used later in the code without checking if it was set up correctly. This could potentially lead to errors if 'outTrace' was not set up because 'out' or 'trace' were falsy. Please ensure that 'outTrace' is always correctly set up. 🧐
generated by pr-review-commit
conditional_trace_setup
@@ -234,7 +264,6 @@ export async function runScript( | |||
if (result.status !== "success") | |||
logVerbose(result.statusText ?? result.status) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The line of code that writes the trace content to 'outTrace' has been removed. If 'outTrace' is expected to contain the trace content, this could lead to unexpected behavior. Please ensure that the trace content is written to 'outTrace' as expected. 🕵️♀️
generated by pr-review-commit
removed_trace_write
The changes introduced in the GIT_DIFF are primarily related to the management of tracing in the codebase.
runScript
appears to be handling loading scripts and preserving trace information.ensureDir
), and to clean up the “out” directory is if theremoveOut
flag is set (emptyDir
).TRACE_CHUNK
event in thetrace
object. When this event fires, the trace chunk details are appended to the file specified byoutTrace
.TRACE_CHUNK
listener logic is copied for a new file "res.trace.md" set in the "out" directory, barring circumstances where theoutTrace
file is the same as "res.trace.md".outTrace
file at the end of the function (writeText(outTrace, trace.content)
) has now been removed to reflect the new approach.